packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py (2)

46-55: Consider a more consistent approach to asynchronous execution.

The _setup_snmp method checks for a running event loop and creates one if missing, but the overall design still seems synchronous in certain places (e.g., in power_cycle). Consider standardizing on an async architecture to avoid mixed paradigms, especially if you plan to integrate with other async code.

🧰 Tools

🪛 GitHub Actions: Run Tests

[error] Failed to spawn: pytest. Caused by: No such file or directory (os error 2)

---

163-167: Optional cleanup or resource management step might be beneficial.

close() is currently a no-op but does call super().close() if defined. In the event that future changes introduce persistent transports or caches, consider finalizing or cleaning them in this method.

🧰 Tools

🪛 GitHub Actions: Run Tests

[error] Failed to spawn: pytest. Caused by: No such file or directory (os error 2)

packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)

24-48: Enhance error reporting in CLI commands.

Currently, the CLI subcommands (on, off, cycle) print only the returned result. If unexpected exceptions occur in the SNMP driver, consider capturing and displaying them with more context or exit codes to improve troubleshooting.

🧰 Tools

🪛 GitHub Actions: Run Tests

[error] Failed to spawn: pytest. Caused by: No such file or directory (os error 2)

packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py (1)

150-156: 🛠️ Refactor suggestion

Avoid blocking calls inside async workflows.

time.sleep(self.quiescent_period) blocks the entire thread. If asynchronous usage is intended, consider replacing it with await asyncio.sleep(self.quiescent_period) and turning power_cycle into an async function for true non-blocking behavior.

- time.sleep(self.quiescent_period)
+ import asyncio
+ await asyncio.sleep(self.quiescent_period)

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools

🪛 GitHub Actions: Run Tests

[error] Failed to spawn: pytest. Caused by: No such file or directory (os error 2)

packages/jumpstarter-driver-snmp/pyproject.toml (2)

12-16: Dependencies Section is Well-Defined

Dependencies for the project are properly declared. The explicit pinning of pysnmp to version 7.1.16 ensures consistency; however, if you intend to permit non-breaking updates, consider using a version specifier like ~=7.1.16 or a minimum version constraint.

---

18-26: Dev Dependency Version Constraints

Dev dependencies are set up correctly under [dependency-groups]. However, note that constraints such as "pytest-anyio>=0.0.0" and "pytest-asyncio>=0.0.0" are effectively unbounded. Specifying a more meaningful minimum version can help avoid unexpected issues in the future.

packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)

11-18: Document the inherited cycle() method.

Since PowerClient now provides a cycle() method, consider adding documentation for this inherited functionality to make the API more discoverable.

 class SNMPServerClient(PowerClient):
     """Client interface for SNMP Power Control"""
+
+    The following methods are available:
+    - on(): Turn power on
+    - off(): Turn power off
+    - cycle(quiescent_period: int = 2): Power cycle the device with a configurable wait period

packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver_test.py (1)

97-125: Add test coverage for error handling scenarios.

The test suite would benefit from additional test cases covering:

1. Hostname resolution failures
2. SNMP timeout scenarios
3. Error responses from the SNMP server

Here's a suggested test case for hostname resolution:

def test_hostname_resolution_failure():
    """Test behavior when hostname resolution fails"""
    with patch('socket.gethostbyname', side_effect=socket.gaierror), \
         pytest.raises(SNMPError, match="Failed to resolve hostname"):
        SNMPServer(
            host="invalid.host",
            user="testuser",
            plug=1
        )

packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py (1)

109-123: Avoid hardcoded target names in SNMP configuration.

The target names "my-creds" and "my-target" should be configurable or use more meaningful defaults.

+    target_params_name: str = field(default="snmp-params")
+    target_address_name: str = field(default="snmp-target")
+
     def _setup_snmp(self):
         # ...
         config.add_target_parameters(
             snmp_engine,
-            "my-creds",
+            self.target_params_name,
             self.user,
             security_level
         )
         config.add_target_address(
             snmp_engine,
-            "my-target",
+            self.target_address_name,
             udp.DOMAIN_NAME,
             (self.ip_address, self.port),
-            "my-creds",
+            self.target_params_name,
             timeout=int(self.timeout * 100),
         )

packages/jumpstarter-driver-snmp/examples/exporter.yaml (1)

7-10: TLS and Token Configuration
The TLS block is set to use an insecure connection (with an empty CA), and the token is a placeholder. While this may be acceptable for testing or development, ensure that secure values are used in production environments.

docs/source/api-reference/drivers/snmp.md (2)

25-39: Detailed Parameter Documentation
The table of configuration parameters is well-structured and informative. For added clarity, consider including an additional column with example values or brief usage notes, which could further aid users in understanding how to appropriately set each parameter.

---

56-64: Cycle Method Documentation
The documentation for cycle(quiescent_period: int = 2) effectively describes the method’s functionality and parameters. It might be useful to elaborate on the operational significance of the default 2-second quiescent period if it has implications on device behavior.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)

11-17: 🛠️ Refactor suggestion

Add missing power control functionality.

Based on the past review comments:

1. Add a cycle method (instead of power_cycle) to align with on/off naming.
2. Consider implementing power state reading functionality.

Here's a suggested implementation:

+    def cycle(self, wait_time: float = 2.0):
+        """Cycle power with configurable wait time
+        
+        Args:
+            wait_time: Time to wait between power off and on in seconds
+        """
+        self.call("cycle", wait_time=wait_time)
+
+    def read_power(self) -> bool:
+        """Read current power state
+        
+        Returns:
+            bool: True if powered on, False if powered off
+        """
+        return self.call("read_power")

packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (2)

19-28: Enhance CLI docstring.

The CLI docstring could be more descriptive to better explain the available commands and their usage.

-            """SNMP power control commands"""
+            """SNMP power control commands
+            
+            Provides commands for controlling power via SNMP:
+              - on: Turn power on
+              - off: Turn power off
+              - cycle: Cycle power with configurable wait time
+            """

---

11-17: Add SNMP-specific error handling.

The power control methods should handle SNMP-specific errors to provide meaningful error messages to users.

     def on(self):
         """Turn power on"""
-        self.call("on")
+        try:
+            self.call("on")
+        except Exception as e:
+            raise SNMPError(f"Failed to turn power on: {str(e)}")

     def off(self):
         """Turn power off"""
-        self.call("off")
+        try:
+            self.call("off")
+        except Exception as e:
+            raise SNMPError(f"Failed to turn power off: {str(e)}")

+class SNMPError(Exception):
+    """Exception raised for SNMP-specific errors"""
+    pass

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py (2)

7-12: Add docstrings to improve test clarity.

While the test implementation is clean, adding docstrings would help explain:

1. The purpose of the SimpleDriver test class
2. The test scenario being validated
3. The expected behavior

 def test_composite_basic():
+    """Test basic functionality of the Composite driver pattern.
+    
+    Verifies that:
+    - Multiple drivers can be added as children
+    - Children are correctly accessible by their keys
+    - The composite maintains the expected number of children
+    """
     class SimpleDriver(Driver):
+        """A minimal Driver implementation for testing the composite pattern."""
         @classmethod
         def client(cls) -> str:
             return "test.client.SimpleClient"

---

13-23: Enhance test coverage with additional scenarios.

While the basic functionality is well tested, consider adding test cases for:

1. Error handling (e.g., duplicate keys, invalid children)
2. Power control operations to verify proper delegation to child drivers
3. Edge cases (e.g., empty composite, single child)

Example test cases to add:

def test_composite_error_cases():
    """Test error handling in the Composite driver."""
    child = SimpleDriver()
    
    # Test duplicate keys
    with pytest.raises(ValueError):
        Composite(children={
            "child1": child,
            "child1": SimpleDriver()  # Duplicate key
        })
    
    # Test empty composite
    empty = Composite(children={})
    assert len(empty.children) == 0

def test_composite_power_control():
    """Test power control operations in composite driver."""
    # Implement power control test cases based on the
    # SNMP driver functionality being added in this PR

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py (1)

7-12: Consider moving SimpleDriver class outside the test function.

While defining test-specific classes within test functions is common, moving SimpleDriver outside could improve reusability across other test cases.

+class SimpleDriver(Driver):
+    @classmethod
+    def client(cls) -> str:
+        return "test.client.SimpleClient"
+
 def test_composite_basic():
-    class SimpleDriver(Driver):
-        @classmethod
-        def client(cls) -> str:
-            return "test.client.SimpleClient"

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/jumpstarter/jumpstarter/config/exporter_test.py (1)

56-56: Enhance test coverage with state verification.

While removing the "ok" assertion aligns with the interface standardization, consider adding state verification after the power control operation to ensure the command was executed successfully. Additionally, consider adding a test for the power off operation for completeness.

Here's a suggested enhancement:

     async with lease.connect_async() as client:
-        await client.power.call_async("on")
+        await client.power.call_async("on")
+        state = await client.power.call_async("read")
+        assert state == "on"
+        
+        await client.power.call_async("off")
+        state = await client.power.call_async("read")
+        assert state == "off"
         assert hasattr(client.nested, "tcp")

docs/source/api-reference/drivers/snmp.md (2)

7-23: YAML Configuration Block
The YAML code block (lines 7–23) is well-structured and clearly lists all required configuration parameters for the SNMP driver. Each parameter is presented with its expected datatype and default value. One minor suggestion: if the document is intended for public consumption, consider adding a note about not exposing sensitive information in production configurations.

---

25-39: Configuration Parameters Table Review
The parameters table (lines 25–39) is detailed and provides a clear understanding of each configuration setting. As a nitpick, you might want to clarify whether fields marked as null (e.g., auth_key, priv_key) require special handling in the driver logic or if they are simply omitted.

packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py (3)

14-14: Consider maintaining return type hints for interface methods.

The removal of return type annotations from abstract methods could lead to inconsistent implementations. For better interface definition and type safety, consider keeping or adding appropriate return type hints.

-    async def on(self): ...
+    async def on(self) -> None: ...

-    async def off(self): ...
+    async def off(self) -> None: ...

Also applies to: 17-17

---

29-30: Add state transition validation.

The current implementation allows any state transition without validation. Consider adding checks to ensure valid state transitions and handle edge cases.

     async def on(self):
+        if self._power_state == PowerState.ON:
+            return  # Already on
-        self._power_state = "on"
+        self._power_state = PowerState.ON

     async def off(self):
+        if self._power_state == PowerState.OFF:
+            return  # Already off
-        self._power_state = "off"
+        self._power_state = PowerState.OFF

Also applies to: 33-34

---

42-46: Add documentation to clarify synchronous implementation.

The class name suggests a synchronous implementation, but it would be helpful to add docstring documentation explaining when to use this vs the async version.

 class SyncMockPower(PowerInterface, Driver):
+    """Synchronous implementation of PowerInterface for testing purposes.
+    
+    Use this implementation when working in a synchronous context.
+    For asynchronous contexts, use MockPower instead.
+    """

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py (1)

29-30: 🛠️ Refactor suggestion

Use an enum for power states instead of strings.

Using string literals for state management is error-prone.

Also applies to: 33-34

packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py (1)

43-45: Reduce code duplication with MockPower.

The state management code is duplicated between MockPower and SyncMockPower. Consider extracting common functionality into a base class.

class BaseMockPower:
    def __init__(self):
        self._power_state = PowerState.OFF
    
    def _set_power_state(self, state: PowerState) -> None:
        self._power_state = state

class MockPower(BaseMockPower, PowerInterface, Driver):
    async def on(self) -> None:
        self._set_power_state(PowerState.ON)
    
    async def off(self) -> None:
        self._set_power_state(PowerState.OFF)

class SyncMockPower(BaseMockPower, PowerInterface, Driver):
    def on(self) -> None:
        self._set_power_state(PowerState.ON)
    
    def off(self) -> None:
        self._set_power_state(PowerState.OFF)

Also applies to: 48-49, 52-53

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)

17-24: Enhance cycle() implementation with error handling and async support.

The current implementation has potential issues that were previously identified:

1. Uses blocking time.sleep() which could be problematic in async contexts
2. No error handling for failed operations
3. Incomplete docstring
4. Missing return type annotation

Consider this enhanced implementation:

-    def cycle(self, wait: int = 2):
-        """Power cycle the device"""
+    def cycle(self, wait: int = 2) -> None:
+        """Power cycle the device
+        
+        Args:
+            wait: Time to wait between power off and on in seconds
+            
+        Raises:
+            Exception: If power off or on operations fail
+        """
         self.logger.info("Starting power cycle sequence")
-        self.off()
-        self.logger.info(f"Waiting {wait} seconds...")
-        time.sleep(wait)
-        self.on()
-        self.logger.info("Power cycle sequence complete")
+        try:
+            self.off()
+            self.logger.info(f"Waiting {wait} seconds...")
+            if asyncio.get_event_loop().is_running():
+                await asyncio.sleep(wait)
+            else:
+                time.sleep(wait)
+            self.on()
+            self.logger.info("Power cycle sequence complete")
+        except Exception as e:
+            error_msg = f"Power cycle failed: {str(e)}"
+            self.logger.error(error_msg)
+            raise

packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)

46-51: Add input validation for wait time.

Consider adding validation to ensure the wait time is within reasonable bounds:

         @base.command()
-        @click.option('--wait', '-w', default=2, help='Wait time in seconds between off and on')
+        @click.option('--wait', '-w', default=2, type=click.IntRange(min=1, max=60),
+                      help='Wait time in seconds between off and on (1-60)')
         def cycle(wait):
             """Power cycle"""
             click.echo(f"Power cycling with {wait} seconds wait time...")
             self.cycle(wait)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py (1)

2-5: Fix import issues and follow Python import conventions.

The import block has several issues:

1. Driver import is unused
2. MockPower import is missing but used in the test
3. Import block needs sorting according to PEP 8

Apply this diff to fix the imports:

-from jumpstarter_driver_composite.driver import Composite
-
-from jumpstarter.driver import Driver
-from jumpstarter.common.utils import serve
+from jumpstarter.common.utils import serve
+from jumpstarter_driver_composite.driver import Composite
+from jumpstarter_driver_power.driver import MockPower

🧰 Tools

🪛 Ruff (0.8.2)

4-4: jumpstarter.driver.Driver imported but unused

Remove unused import: jumpstarter.driver.Driver

(F401)

🪛 GitHub Check: ruff

[failure] 2-7: Ruff (I001)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py:2:1: I001 Import block is un-sorted or un-formatted

---

[failure] 4-4: Ruff (F401)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py:4:32: F401 jumpstarter.driver.Driver imported but unused

🪛 GitHub Actions: Lint

[error] 2-2: Import block is un-sorted or un-formatted.

packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)

17-24: Enhance cycle implementation with error handling and async support.

The current implementation needs improvements in several areas:

1. Uses blocking time.sleep in a potentially async context
2. Lacks error handling for failed operations
3. Missing parameter documentation

Previous review suggested a more robust implementation with error handling and async support. Consider implementing those improvements:

-    def cycle(self, wait: int = 2):
-        """Power cycle the device"""
-        self.logger.info("Starting power cycle sequence")
-        self.off()
-        self.logger.info(f"Waiting {wait} seconds...")
-        time.sleep(wait)
-        self.on()
-        self.logger.info("Power cycle sequence complete")
+    async def cycle(self, wait: int = 2):
+        """Power cycle the device
+        
+        Args:
+            wait: Time to wait between power off and on in seconds
+        
+        Raises:
+            Exception: If power off or on operations fail
+        """
+        try:
+            self.logger.info("Starting power cycle sequence")
+            await self.off()
+            self.logger.info(f"Waiting {wait} seconds...")
+            await asyncio.sleep(wait)
+            await self.on()
+            self.logger.info("Power cycle sequence complete")
+        except Exception as e:
+            error_msg = f"Power cycle failed: {str(e)}"
+            self.logger.error(error_msg)
+            raise

packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)

46-51: Enhance CLI command with error handling and async support.

The CLI integration could be improved with proper error handling and async support.

Consider this enhanced implementation:

         @base.command()
         @click.option('--wait', '-w', default=2, help='Wait time in seconds between off and on')
-        def cycle(wait):
+        async def cycle(wait):
             """Power cycle"""
-            click.echo(f"Power cycling with {wait} seconds wait time...")
-            self.cycle(wait)
+            try:
+                click.echo(f"Power cycling with {wait} seconds wait time...")
+                await self.cycle(wait)
+                click.echo("Power cycle completed successfully")
+            except Exception as e:
+                click.echo(f"Error during power cycle: {str(e)}", err=True)
+                raise click.Abort()

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

docs/source/api-reference/drivers/snmp.md (2)

7-23: Add comments to highlight default values in the configuration example.

Consider adding inline comments to indicate which parameters are optional and their default values. This would make the configuration example more user-friendly.

 export:
   power:
     type: "jumpstarter_driver_snmp.driver.SNMPServer"
     config:
       host: "pdu.mgmt.com"
       user: "labuser"
       plug: 32
-      port: 161
+      port: 161  # Optional, default: 161
-      oid: "1.3.6.1.4.1.13742.6.4.1.2.1.2.1"
+      oid: "1.3.6.1.4.1.13742.6.4.1.2.1.2.1"  # Optional, default: "1.3.6.1.4.1.13742.6.4.1.2.1.2.1"
-      auth_protocol: "NONE"
+      auth_protocol: "NONE"  # Optional, default: "NONE"
       auth_key: null
-      priv_protocol: "NONE"
+      priv_protocol: "NONE"  # Optional, default: "NONE"
       priv_key: null
-      timeout: 5.0
+      timeout: 5.0  # Optional, default: 5.0

---

34-37: Enhance security parameter descriptions.

The descriptions for authentication and privacy parameters could be more detailed to help users make informed security decisions.

-| auth_protocol | Authentication protocol ("NONE", "MD5", "SHA") | str | no | "NONE" |
-| auth_key | Authentication key when auth_protocol is not "NONE" | str | no | null |
-| priv_protocol | Privacy protocol ("NONE", "DES", "AES") | str | no | "NONE" |
-| priv_key | Privacy key when priv_protocol is not "NONE" | str | no | null |
+| auth_protocol | Authentication protocol: "NONE" (no auth), "MD5" (HMAC-MD5-96), "SHA" (HMAC-SHA-96) | str | no | "NONE" |
+| auth_key | Authentication passphrase (8+ chars) required when auth_protocol is not "NONE" | str | no | null |
+| priv_protocol | Privacy (encryption) protocol: "NONE" (no privacy), "DES" (CBC-DES), "AES" (CFB-AES-128) | str | no | "NONE" |
+| priv_key | Privacy passphrase (8+ chars) required when priv_protocol is not "NONE" | str | no | null |

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro